Skip to content

fix(pipeline): guard GitHub fork PR destinations - #891

Open
tiago-peixoto wants to merge 15 commits into
kunchenguid:mainfrom
tiago-peixoto:fm/no-mistakes-pr-destination-guard
Open

fix(pipeline): guard GitHub fork PR destinations#891
tiago-peixoto wants to merge 15 commits into
kunchenguid:mainfrom
tiago-peixoto:fm/no-mistakes-pr-destination-guard

Conversation

@tiago-peixoto

@tiago-peixoto tiago-peixoto commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Intent

Superseding Decisions

Later recorded decisions take precedence over conflicting wording in the original intent below. Within this list, later entries supersede earlier conflicting entries.

  • Authoritative, structure-preserved explicit intent from no-mistakes axi run --intent, no-mistakes rerun --intent, or the existing notify-push push-option intent may authorize publication when PR destination: owner/repo matches the selected parent.

  • An intermediate review decision restricted fork-PR authorization to AXI provenance and rejected the direct-push intent transport.

  • The final decision restores the first rule: authoritative, structure-preserved explicit intent from AXI, rerun, or notify-push may authorize publication. No AXI-provenance machinery remains.

The final entry is authoritative. It also corrects the earlier cost disclosure: it was overstated to say that direct pushes necessarily validate and push before refusing. Only a bare direct push without authoritative intent takes that late-refusal path; a direct push carrying the existing authoritative notify-push intent can authorize publication. The earlier AXI-only wording in this PR body and review discussion is superseded.

What Changed

  • Before any GitHub PR lookup, update, or creation, require the selected parent repository to agree exactly with an explicit PR destination: owner/repo declaration carried by authoritative, structure-preserved run intent.
  • Refuse caller-visibly when the destination is missing, malformed, ambiguous, or disagrees with the selected parent, so a parent-plus-fork registration cannot silently default to a public parent PR.
  • Preserve the supported fork-head-to-parent-PR path when the explicit destination is the parent, preserve unaffected non-fork behavior, and keep provider-unavailable runs on the existing skip path.
  • Preserve exact decision chronology in generated PR bodies: decisions are ordered globally by created_at and stable ID, and repeated A -> B -> A reversals keep the final A.

Closes #884.

Maintainer Reproduction

The maintainer reproduced the defect on current main at 13cdf1aac70e7cd07525e1729f3dd2399bb76217: buildHost in internal/pipeline/steps/host.go selected GitHub --repo from Repo.UpstreamURL, used Repo.ForkURL only for the --head owner, and PRStep could call CreatePR without checking that repository choice against the run's delivery intent. This change implements that already-established diagnosis.

In our own contribution workflow, we do not yet carry a local automatic guard ourselves; we currently check the target manually.

Observable Publication Contract

  • Destination validation runs after provider availability but before drafting or any pr list, pr edit, or pr create operation. If gh is absent or unauthenticated, the existing provider-unavailable skip remains unchanged.
  • A mismatched fork destination and every unknown, malformed, ambiguous, or legacy structure-unproven destination refuse before publication.
  • An explicit parent destination still creates the parent PR with <fork-owner>:<branch> as its head, preserving the upstream-contribution path from fix(scm): no-mistakes cannot ship fork-based contributions — push and PR share one upstream_url, no fork→parent routing #293.
  • A bare direct push has no authoritative destination. It may complete validation and push the branch to the fork, then refuse caller-visibly at the PR step. This is intentionally a late publication guard; no earlier push-time failure was implemented.
  • axi run --intent, rerun --intent, and the existing notify-push push-option intent are all accepted when they preserve the explicit leading destination structure. Repository topology, issue links, incidental prose, and transcript text do not grant authorization.

Test Evidence

The narrow regression was added first and run against unmodified 13cdf1aac70e7cd07525e1729f3dd2399bb76217 with:

GIT_CONFIG_GLOBAL=/dev/null go test ./internal/pipeline/steps -run 'TestPRStep_GitHubFork(RefusesUnverifiedDestination|CreatesParentPRWithForkHead)$' -count=1

Red before: the mismatch, unknown, and ambiguous cases failed with Execute() error = nil, and the fake GitHub CLI transcript proved PR creation remained reachable. Green after: the same command passed, proving refusal before PR operations and preserving the positive explicit-parent case.

Two executable end-to-end proofs also pass:

go test -tags=e2e ./internal/e2e -run '^(TestForkRouting|TestForkRoutingDirectPushRefusesPRWithoutDestination)$' -count=1 -v

TestForkRouting proves an authoritative explicit parent destination completes with a fork push and parent PR. TestForkRoutingDirectPushRefusesPRWithoutDestination proves a bare direct push completes validation and fork push, then refuses before PR publication. The focused PR-step suite additionally covers the provider-unavailable skip, literal .git repository names, mismatch/unknown/ambiguous refusal, explicit-parent publication, and generated decision chronology.

For the body-history correction, reverting only the chronology implementation reproduced the old A -> B rendering with the final A missing. At 17a39993e318008f859311ccc0242d4e035d2a1c, the focused generated-body test renders A -> B -> A and exactly one live pipeline attestation.

Broad validation completed with make lint, go test ./..., make e2e, and go build -o ./bin/no-mistakes ./cmd/no-mistakes. The managed no-mistakes review, test, document, lint, and push stages completed for exact pushed head 17a39993e318008f859311ccc0242d4e035d2a1c. This PR does not claim a green local go test -race ./...; two attempts hit the repository-documented macOS fork/pre-exec failure in unrelated existing tests.

Scope

This deliberately does not implement issue #552's branch/base binding, add a new push-time destination channel, add authorization/provenance machinery, or redesign unrelated SCM routing. The declaration remains owner/repo; host-qualified destination authorization is a separate follow-up.

Pipeline

Updates from git push no-mistakes

  • Review: completed with no remaining findings.
  • Test: completed; focused unit, generated-body, and fork-routing E2E proofs passed.
  • Document and lint: completed.
  • Push: exact head 17a39993e318008f859311ccc0242d4e035d2a1c published from tiago-peixoto:fm/no-mistakes-pr-destination-guard.

@greptile-apps

greptile-apps Bot commented Aug 29, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Reviews (2): Last reviewed commit: "no-mistakes(review): Order superseding P..." | Re-trigger Greptile

Comment thread internal/pipeline/steps/pr.go
@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate:

This is a captain-decision hold, not waiting on you.

Tip 17a39993 is otherwise ready: MERGEABLE CLEAN, CI green (check / tests / e2e / docs / generated-files), Greptile 5/5. The Greptile P1 about a gate push no longer authorizing a fork PR is the intended #884 contract: refuse when the destination is missing or ambiguous; an explicit parent destination still opens the parent PR. TestForkRoutingDirectPushRefusesPRWithoutDestination matches that.

We will not auto-merge. Unconfigured GitHub fork-mode used to raise the parent PR on a gate push (#293). This tip requires a leading PR destination: owner/repo on authoritative intent, and a bare git push no-mistakes may validate and push to the fork, then refuse PR publication. That is new-default / pipeline-shape even though the motive is the fork-only publication bug. A VISION motive does not turn a replaced default path into restore.

contract-class: new-default

VISION (tip 17a39993):

  • R1 cannot tell (fail-closed destination aligns with refuse-loudly; splitting "push authorizes PR" for fork-mode direct push sits against the consent-boundary sentence)
  • R2 aligns (refuse before any PR list/edit/create)
  • R3 aligns (do not auto-choose parent against stated or unknown destination)
  • R4 cannot tell (no review-role change)
  • R5 aligns (caller-visible refusal; destination inspectable)
  • R6 aligns
  • R7 aligns (strengthens refusal before publish)

@tiago-peixoto

Copy link
Copy Markdown
Contributor Author

Correction to #891 (comment): that inline reply is superseded. The accepted contract does not require AXI-only provenance and does not reject the existing direct-push intent transport. Authoritative, structure-preserved explicit intent from no-mistakes axi run --intent, no-mistakes rerun --intent, or the existing notify-push push-option may authorize fork-head-to-parent publication when PR destination: owner/repo matches the selected parent. Only a bare direct push without authoritative intent takes the late path that validates and pushes to the fork, then refuses before any PR operation. The rejected provenance machinery was removed; no new authorization/provenance layer is part of the final head 17a39993e318008f859311ccc0242d4e035d2a1c. The PR body now records the superseding A -> B -> A decision history and binds its pipeline attestation to that exact head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fork mode can open parent-repo PRs despite explicit fork-only delivery intent

2 participants